Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[various] Lamp handlers expose min/max Color Temperature in state description #17641

Merged
merged 8 commits into from
Nov 1, 2024

Conversation

andrewfg
Copy link
Contributor

@andrewfg andrewfg commented Oct 27, 2024

Lamp handlers expose min/max Color Temperature in state description

Resolves #17638

Signed-off-by: AndrewFG [email protected]

@andrewfg andrewfg added enhancement An enhancement or new feature for an existing add-on additional testing preferred The change works for the pull request author. A test from someone else is preferred though. awaiting other PR Depends on another PR labels Oct 27, 2024
@andrewfg andrewfg requested a review from lolodomo October 27, 2024 18:37
@andrewfg andrewfg self-assigned this Oct 27, 2024
Signed-off-by: AndrewFG <[email protected]>
Copy link
Contributor

@stefan-hoehn stefan-hoehn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

govee lgtm

@lolodomo
Copy link
Contributor

As discussed in the hue PR, there is a better option for the provider.
Let's wait for the merge of PR #17637 first.

@andrewfg andrewfg changed the title Lamp handlers expose min/max Colour Temperature in state description [various] Lamp handlers expose min/max Colour Temperature in state description Oct 28, 2024
Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you

@andrewfg andrewfg removed the awaiting other PR Depends on another PR label Oct 28, 2024
@lolodomo
Copy link
Contributor

@andrewfg : what kind of tests exactly are you waiting for?

@andrewfg
Copy link
Contributor Author

what kind of tests exactly are you waiting for?

Ideally I would like 1) to wait for your CT widget and then 2) ask users to test the min/max correct range.
However in absence of both 1) and 2) perhaps I should write some JUnit tests to test on a mocked thing / channel?

Copy link
Member

@wborn wborn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

US English would be preferred.

Signed-off-by: AndrewFG <[email protected]>
@andrewfg andrewfg changed the title [various] Lamp handlers expose min/max Colour Temperature in state description [various] Lamp handlers expose min/max Color Temperature in state description Oct 29, 2024
@andrewfg
Copy link
Contributor Author

perhaps I should write some JUnit tests to test on a mocked thing / channel?

@lolodomo I spent some hours trying to write proper Junit tests with mocks etc. but in the end it would require modifying the bindings too substantially and risks to break more than it fixes. So instead, I have done some local actual and thought experiments on the code. And as a result I made some minor improvements. And I am now convinced that it is ready to merge.

@andrewfg andrewfg removed the additional testing preferred The change works for the pull request author. A test from someone else is preferred though. label Oct 31, 2024
@andrewfg andrewfg requested a review from lolodomo October 31, 2024 16:44
Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you

@lolodomo lolodomo merged commit 2080c85 into openhab:main Nov 1, 2024
5 checks passed
@lolodomo lolodomo added this to the 4.3 milestone Nov 1, 2024
@andrewfg andrewfg deleted the color-temp-set-min-max branch November 6, 2024 15:45
KaaNee pushed a commit to KaaNee/openhab-addons that referenced this pull request Nov 8, 2024
…cription (openhab#17641)

* Lamp handlers expose min/max Colour Temperature in state description
* add color temperature validit and range checks

Signed-off-by: AndrewFG <[email protected]>
matchews pushed a commit to matchews/openhab-addons that referenced this pull request Dec 16, 2024
…cription (openhab#17641)

* Lamp handlers expose min/max Colour Temperature in state description
* add color temperature validit and range checks

Signed-off-by: AndrewFG <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add min/max color temperature capabilities to channel state descriptions
4 participants